Prevent logptest.NewTestingLogger from panics. #368
Conversation
Because NewTestingLogger used testing.T as the logger output, if the logger is used after the test has ended, it will panic. This behaviour has been seen a few times and causes flakiness in our CI. To mitigate this, NewTestingLogger now just calls NewFileLogger that uses a file as the logger output, thus being safe for use even after the test has ended.
belimawr
requested review from
AndersonQ,
orestisfl and
rdner
and removed request for
a team
💔 Build Failed
Failed CI Steps
History |
pierrehilbert
added
the
Team:Elastic-Agent-Data-Plane
| // DEPRECATED: The logger returned by [NewTestingLogger] can panic if it is | ||
| // used after the test has ended. Use [NewFileLogger] instead. |
There was a problem hiding this comment.
Please don't do this, the testing logger is integrated in the stdlib testing infra.
The panic is exposing a bug in the code, that's where it should be fixed.
There was a problem hiding this comment.
I created elastic/beats#47527 to track the failing tests because of the logger issue.
|
I honestly think this PR is unnecessary. The panic occurs because there are calls to |
|
Thanks for the feedback folks, I'll close the PR. |
5 participants
What does this PR do?
Because NewTestingLogger used testing.T as the logger output, if the logger is used after the test has ended, it will panic.
This behaviour has been seen a few times and causes flakiness in our CI.
To mitigate this, NewTestingLogger now just calls NewFileLogger that uses a file as the logger output, thus being safe for use even after the test has ended.
Why is it important?
It prevents tests from becoming flaky
Checklist
I have added tests that prove my fix is effective or that my feature works## Author's Checklist## Related issues